-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adapting lansuite to abide STRICT_TRANS_TABLES #458
base: master
Are you sure you want to change the base?
Conversation
if the database uses a strict setting STRICT_TRANS_TABLES (https://mariadb.com/kb/en/sql-mode/#strict_trans_tables) then incorrect queries (empty string instead of zero) will result in an error. STRICT_TRANS_TABLES is part of the defaults in mariadb and mysql https://mariadb.com/kb/en/sql-mode/#defaults https://dev.mysql.com/doc/refman/8.0/en/sql-mode.html#sql-mode-strict - Fix setting int and enum correctly in UPDATE/DELETE query in MasterForm - Setting more reasonable default valuse (date, datetime, time never had defaults set and have been removed from db.xml) - field module in ip_locklist was increased in size as it was to small for some module names
Still pending ..... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this Pull Request, and sorry for letting it slip so long.
I added a few questions - Let me know what you think.
inc/Classes/MasterForm.php
Outdated
@@ -1117,14 +1117,18 @@ public function SendForm($BaseURL, $table, $idname = '', $id = 0) | |||
foreach ($this->SQLFields as $key => $val) { | |||
if (($SQLFieldTypes[$val] == 'datetime' or $SQLFieldTypes[$val] == 'date') and $_POST[$val] == 'NOW()') { | |||
$db_query .= "$val = NOW(), "; | |||
} elseif ($SQLFieldTypes[$val] == 'tinyint(1)') { | |||
} elseif ($this->is_field_int($SQLFieldTypes[$val])) { | |||
$db_query .= $val .' = '. (int)$_POST[$val] .', '; | |||
} elseif ($SQLFieldTypes[$val] == 'varbinary(16)' and $val == 'ip') { | |||
$db_query .= $val .' = INET6_ATON(\''. $_POST[$val] .'\'), '; | |||
} elseif ($_POST[$val] == '++' and strpos($SQLFieldTypes[$val], 'int') !== false) { | |||
$db_query .= "$val = $val + 1, "; | |||
} elseif ($_POST[$val] == '--' and strpos($SQLFieldTypes[$val], 'int') !== false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this check also benefit from is_field_int
?
inc/Classes/MasterForm.php
Outdated
@@ -1117,14 +1117,18 @@ public function SendForm($BaseURL, $table, $idname = '', $id = 0) | |||
foreach ($this->SQLFields as $key => $val) { | |||
if (($SQLFieldTypes[$val] == 'datetime' or $SQLFieldTypes[$val] == 'date') and $_POST[$val] == 'NOW()') { | |||
$db_query .= "$val = NOW(), "; | |||
} elseif ($SQLFieldTypes[$val] == 'tinyint(1)') { | |||
} elseif ($this->is_field_int($SQLFieldTypes[$val])) { | |||
$db_query .= $val .' = '. (int)$_POST[$val] .', '; | |||
} elseif ($SQLFieldTypes[$val] == 'varbinary(16)' and $val == 'ip') { | |||
$db_query .= $val .' = INET6_ATON(\''. $_POST[$val] .'\'), '; | |||
} elseif ($_POST[$val] == '++' and strpos($SQLFieldTypes[$val], 'int') !== false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this check also benefit from is_field_int
?
@@ -229,7 +233,7 @@ public function ImportXML($rewrite = null) | |||
// Handle structure changes in general | |||
} elseif ($db_field["Type"] != $type | |||
or (!($db_field["Null"] == $null or ($db_field["Null"] == 'YES' and $null == 'NULL'))) | |||
or ($db_field["Default"] != $default_xml and !($db_field["Default"] == 0 and $default_xml == '') and !($db_field["Default"] == '' and $default_xml == 0)) | |||
or ($db_field["Default"] != $default_xml and !($db_field["Default"] == 0 and $default_xml == '') and !($db_field["Default"] == '' and $default_xml === 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this ever work without casting/double checks?
$default_xml
is set by
$default_xml = $this->xml->getFirstTagContent("default", $field);
getFirstTagContent
will always return a string, even is 0
is in it.
Maybe it is better to run something like $default_xml === '0'
?
See
Lines 16 to 41 in 7e16dd2
public function getFirstTagContent($tag, $input, $save = false) | |
{ | |
$start = strpos($input, '<' . $tag . '>') + strlen($tag) + 2; | |
// If the tag has attributes, remove them, for the end tag won't contain them | |
if (strpos($tag, ' ') > 0) { | |
$tag = substr($tag, 0, strpos($tag, ' ')); | |
} | |
$end = strpos($input, '</' . $tag . '>') - $start; | |
if ($end <= 0) { | |
return ''; | |
} | |
if ($save) { | |
return trim( | |
str_replace( | |
'--lt--', | |
'<', | |
str_replace('--gt--', '>', substr($input, $start, $end)) | |
) | |
); | |
} | |
return trim(substr($input, $start, $end)); | |
} |
@@ -116,7 +116,7 @@ | |||
<type>datetime</type> | |||
<null></null> | |||
<key></key> | |||
<default>0</default> | |||
<default></default> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate a bit why removing the default is necessary here? (same question applies to the other datetime fields)
@@ -98,7 +98,7 @@ | |||
<type>datetime</type> | |||
<null></null> | |||
<key></key> | |||
<default></default> | |||
<default>0000-00-00 00:00:00</default> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes this datetime field different from the others when it comes to the default value?
@@ -333,7 +333,7 @@ | |||
<field> | |||
<name>rules</name> | |||
<type>text</type> | |||
<null></null> | |||
<null>YES</null> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this Pull Request, and sorry for letting it slip so long.
I added a few questions - Let me know what you think.
FYI: #634 was implemented to mitigate this issue slightly. Making LanSuite support |
Co-authored-by: Andy Grunwald <andy.grunwald@aiven.io>
Co-authored-by: Andy Grunwald <andy.grunwald@aiven.io>
Co-authored-by: Andy Grunwald <andy.grunwald@aiven.io>
Co-authored-by: Andy Grunwald <andy.grunwald@aiven.io>
SonarCloud Quality Gate failed. 22 Bugs No Coverage information |
Please retry analysis of this Pull-Request directly on SonarCloud. |
After 2,5 years, I cannot tell you the reasoning behind all the changes anymore. As far as I remember I made one change after another to make lansuite work |
Thx @t-h-e. Would you mind sharing the URL of your installation? Or is it locally? I plan to look into this more in detail and get this merged soon(ish). I believe it is the right direction. |
URL: https://www.haag-networx.at Activedted modules:
|
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Quality Gate failedFailed conditions |
if the database uses a strict setting
STRICT_TRANS_TABLES
(https://mariadb.com/kb/en/sql-mode/#strict_trans_tables)
then incorrect queries (empty string instead of zero) will result in an
error.
STRICT_TRANS_TABLES
is part of the defaults in mariadb and mysqlhttps://mariadb.com/kb/en/sql-mode/#defaults
https://dev.mysql.com/doc/refman/8.0/en/sql-mode.html#sql-mode-strict
MasterForm
defaults set and have been removed from db.xml)
for some module names
fixes parts of #456